Skip to content

Additional percent-encode sets #837

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Additional percent-encode sets #837

wants to merge 12 commits into from

Conversation

@cryptoquick
Copy link

@cryptoquick cryptoquick commented May 7, 2023 •
edited
Loading

I've added some additional constants that should be useful for others.

I'd also recommend adding #721.

I've also corrected some docs generation warnings.

lucacasonato
lucacasonato reviewed Jul 11, 2023
View reviewed changes
Copy link
Collaborator

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These sets are duplicated across percent_encoding and url/src/parser.rs now.

Can you de-dupe?

@cryptoquick
Copy link
Author

Good point! Will do.

@codecov
Copy link

codecov bot commented Jul 12, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.42%. Comparing base (11bb12f) to head (e2ebbf0).
Report is 3 commits behind head on master.

❗ Current head e2ebbf0 differs from pull request most recent head be685e1. Consider uploading reports for the commit be685e1 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #837      +/-   ##
==========================================
- Coverage   81.75%   81.42%   -0.33%     
==========================================
  Files          20       20              
  Lines        3551     3516      -35     
==========================================
- Hits         2903     2863      -40     
- Misses        648      653       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lucacasonato lucacasonato force-pushed the addl-percent-encode-sets branch from deeeafc to 7cf28a9 Compare July 12, 2023 11:29
@cryptoquick
Copy link
Author

Let me know if there's anything more I need to do in order to help get this in, thanks!

@cryptoquick cryptoquick requested a review from lucacasonato March 2, 2024 18:00
@cyqsimon cyqsimon mentioned this pull request Sep 23, 2024
Merged
@cyqsimon
Copy link

Bump. Had to do some duplicate work in miniserve in lieu of this.

I see in UPGRADING.md that these sets were previously included but have since been removed, but I could not find a justification. Does someone know why this decision was made?

Regardless, I'm in favour of seeing these sets merged.

@joshka joshka mentioned this pull request Sep 27, 2024
Open
@cryptoquick
Copy link
Author

Hi, I just got the PR updated and there seems to be a need for this work, can we please get this merged? I keep finding bits and pieces of it getting duplicated elsewhere now, too. Ping @lucacasonato

@cryptoquick
Copy link
Author

Ping @joshka
Please review, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lucacasonato lucacasonato Awaiting requested review from lucacasonato

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cryptoquick @cyqsimon @lucacasonato